Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: search input a11y issue #679

Merged
merged 1 commit into from
May 13, 2019
Merged

fix: search input a11y issue #679

merged 1 commit into from
May 13, 2019

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented May 10, 2019

Summary

EDIT: See #687

This PR was reverted because of failing test. See the new PR above

#418

The search input does not have aria-label. I have found the underlying cause, it is because we are not passing ariaLabel options to autocomplete.js
See https://github.com/algolia/autocomplete.js/#global-options

ariaLabel - An optional string that will populate the aria-label attribute.

My implementation is

  • Check autoCompleteOptions for ariaLabel
  • Otherwise, check original input selector aria-label
  • Otherwise, use search input as aria-label

Test Plan

Here is my input

<input
  id="search_input_react"
  type="search"
  placeholder="Search"
  aria-label="This is my aria-label from endiliey"
/>

Before
No aria-label
image

After
aria-label exist
aria-label working

@Haroenv Haroenv requested a review from s-pace May 13, 2019 07:27
@s-pace s-pace merged commit a68d2a4 into algolia:master May 13, 2019
@s-pace
Copy link

s-pace commented May 13, 2019

👏 @endiliey , you are now a DocSearch collaborator 🚀

s-pace pushed a commit that referenced this pull request May 13, 2019
s-pace pushed a commit that referenced this pull request May 13, 2019
@s-pace
Copy link

s-pace commented May 13, 2019

Reverting since:

DocSearch  constructor  should allow customize algoliaOptions without loosing default options

    TypeError: Cannot read property 'getAttribute' of undefined

      81 |     this.autocompleteOptions.cssClasses.prefix =
      82 |       this.autocompleteOptions.cssClasses.prefix || 'ds';
    > 83 |    this.autocompleteOptions.ariaLabel = this.autocompleteOptions.ariaLabel || $(this.input)[0].getAttribute('aria-label') || 'search input';
         |                                                                               ^
      84 |
      85 |
      86 |     this.isSimpleLayout = layout === 'simple';

@s-pace
Copy link

s-pace commented May 13, 2019

Do you have an idea @endiliey ?

@endiliey
Copy link
Contributor Author

$(this.input)[0] might be undefined. I should've used a safe getter like

$(this.input)[0] && $(this.input)[0].getAttribute('aria-label')

@s-pace
Copy link

s-pace commented May 13, 2019

Ok, feel free to submit a new PR :)

@endiliey
Copy link
Contributor Author

@s-pace sorry for the delay. was doing something else

see #687

Note that autocomplete.js must be >= v0.36 because autoCompleteOptions.ariaLabel is not available in < 0.36

s-pace pushed a commit that referenced this pull request May 13, 2019
### Bug Fixes

* **deps:** update dependency autocomplete.js to v0.35.0 ([2d67a21](2d67a21))
* **deps:** update dependency autocomplete.js to v0.36.0 ([bbd4ef5](bbd4ef5))
* **security:** Fix dependencies to avoid infected event-stream ([9f93ffb](9f93ffb))
* docsearch input should have aria-label (a11y) ([#687](#687)) ([59d21f5](59d21f5))
* search input a11y issue ([#679](#679)) ([a68d2a4](a68d2a4))
s-pace pushed a commit that referenced this pull request Aug 22, 2019
s-pace pushed a commit that referenced this pull request Aug 22, 2019
s-pace pushed a commit that referenced this pull request Aug 22, 2019
### Bug Fixes

* **deps:** update dependency autocomplete.js to v0.35.0 ([2d67a21](2d67a21))
* **deps:** update dependency autocomplete.js to v0.36.0 ([bbd4ef5](bbd4ef5))
* **security:** Fix dependencies to avoid infected event-stream ([9f93ffb](9f93ffb))
* docsearch input should have aria-label (a11y) ([#687](#687)) ([59d21f5](59d21f5))
* search input a11y issue ([#679](#679)) ([a68d2a4](a68d2a4))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants